delegatingresolver: add default port to addresses #8613

eshitachandwani · 2025-09-27T12:29:29Z

RELEASE NOTES:

Fixes a bug where default port 443 was not being added to addresses without port being sent to proxy.
Adds a new environment variable GRPC_EXPERIMENTAL_RESOLVER_ADD_DEFAULT_PORT for adding a default port to addresses being sent to proxy which is set by default.

codecov · 2025-09-28T13:10:54Z

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.97%. Comparing base (bb71072) to head (82076ed).
⚠️ Report is 17 commits behind head on master.

Files with missing lines	Patch %	Lines
.../resolver/delegatingresolver/delegatingresolver.go	93.10%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8613      +/-   ##
==========================================
- Coverage   82.00%   81.97%   -0.03%     
==========================================
  Files         415      415              
  Lines       40697    40719      +22     
==========================================
+ Hits        33372    33380       +8     
- Misses       5937     5970      +33     
+ Partials     1388     1369      -19

Files with missing lines	Coverage Δ
internal/envconfig/envconfig.go	`100.00% <ø> (ø)`
.../resolver/delegatingresolver/delegatingresolver.go	`87.39% <93.10%> (+0.97%)`	⬆️

... and 51 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

internal/resolver/delegatingresolver/delegatingresolver.go

internal/envconfig/envconfig.go

arjan-bal · 2025-10-03T07:05:46Z

internal/resolver/delegatingresolver/delegatingresolver.go

+	addr, err := parseTarget(target.Endpoint())
+	if err != nil {
+		return nil, fmt.Errorf("delegating_resolver: invalid target address %q: %v", target.Endpoint(), err)
+	}


I think we should guard this call to parseTarget behind the feature flag since it can potentially lead to new/different failures after this PR is merged.

We can have only one block guarded by the feature flag if we instead do the following:

addr := target.Endpoint() if target.URL.Scheme == "dns" && !targetResolutionEnabled && envconfig.AddDefaultPort { addr, err = parseTarget(target.Endpoint(), defaultPort) if err != nil { // return some error } }

This way we can use the parseTarget function from the dns resolver without any modifications.

My thinking was we should do the basic check and add localhost for all addresses if no host is present irrespective of the resolver being used, but adding default port 443 is only for DNS resolver with target resolution disabled. And the first part should not be behind the flag , but only 2nd part should. That is my understanding, let me know if I am missing something.

From my understanding, only the DNS resolver was defaulting the hostname to localhost. Applying this logic to all resolvers would be a behaviour change which could potentially break some custom resolver that isn't expecting this. I would recommend avoiding such a change to be safe.

Also, since we're providing a env variable flag, we should ensure the flag can revert all (if not most) changes in this PR.

internal/resolver/delegatingresolver/delegatingresolver.go

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

arjan-bal · 2025-10-03T07:19:14Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+		name               string
+		target             string
+		wantConnectAddress string
+		wantErrorSubstring string


Can you add another test param for the env variable and tests cases for the env variable being disabled?

I already have a seperate test for it : TestDelegatingResolverEnvVarForDefaultPortDisable

Can we combine the tests using an additional param? That should help reduce duplicate code.

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

easwars · 2025-10-07T23:00:20Z

internal/resolver/delegatingresolver/delegatingresolver.go

+	if target == "" {
+		return "", fmt.Errorf("missing address")
+	}


Is this even possible since we already parse the target URI specified by the user in clientconn.go before we get here?

Ideally, it would be nicer to avoid checking conditions that are not possible, since it would simplify the code.

I dont see an explicit check for missing target in the clientconn.go and this is similar to the check we already have in dns.

easwars · 2025-10-07T23:01:50Z

internal/resolver/delegatingresolver/delegatingresolver.go

+			// If the port field is empty (target ends with colon), e.g. "[::1]:",
+			// this is an error.


Optional: I think it would be better to group these inline comments into a docstring for the function. That way, one can just read that docstring and get to know what the function is doing instead of reading it one piece at a time.

easwars · 2025-10-07T23:04:13Z

internal/resolver/delegatingresolver/delegatingresolver.go

+		return "", fmt.Errorf("missing address")
+	}
+	if host, port, err := net.SplitHostPort(target); err == nil {
+		if port == "" {


Based on the documentation of net.SplitHostPort, the port seems to be mandatory and it will return a missing port in address error in that case. So, I don't think we could have err == nil and port == "".

// SplitHostPort splits a network address of the form "host:port", // "host%zone:port", "[host]:port" or "[host%zone]:port" into host or // host%zone and port.

Please correct me if I'm wrong.

err == nil and port == "" would be case where the target is something like : host: i.e. they have the colon but no port specified after it.

easwars · 2025-10-07T23:09:09Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+		disableDefaultPortEnvVar bool
+	}{
+		{
+			name:               "no port in target",


go/gotip/episodes/117

easwars · 2025-10-07T23:10:05Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

-		t.Fatalf("Unexpected state from delegating resolver. Diff (-got +want):\n%v", diff)
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if test.disableDefaultPortEnvVar {


Why do you have to check this field? Why can't you set the value of the env var to the one specified by the test unconditionally? That way, the code will be immune to changes in the default value of the env var.

Even I had thought of that, but idk why I decided to do it this way then. Changed.

easwars · 2025-10-07T23:11:03Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+			}
+			overrideTestHTTPSProxy(t, envProxyAddr)
+
+			targetResolver := manual.NewBuilderWithScheme("dns")


Don't you have to unregister the original dns resolver and re-register it at the end of the test, for this test to not affect other tests?

easwars · 2025-10-07T23:14:37Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+			targetResolver := manual.NewBuilderWithScheme("dns")
+			target := targetResolver.Scheme() + ":///" + test.target
+			// Set up a manual DNS resolver to control the proxy address resolution.
+			proxyResolver, proxyResolverBuilt := setupDNS(t)


Oh I see that this helper does re-register the original dns resolver. But maybe this section can still be a little more explicit/readable etc. I'm not able to think of the exact way to make this better right now.

This is what we had earlier too..in the tests.

easwars · 2025-10-07T23:15:22Z

internal/resolver/delegatingresolver/delegatingresolver_ext_test.go

+			if test.wantErrorSubstring != "" {
+				// Case 1: We expected an error.
+				if err == nil {
+					t.Fatalf("Delegating resolver created, want error containing %q", test.wantErrorSubstring)
+				}
+				if !strings.Contains(err.Error(), test.wantErrorSubstring) {
+					t.Fatalf("Delegating resolver failed with error %v, want error containing %v", err.Error(), test.wantErrorSubstring)
+				}
+				return
+			}
+
+			// Case 2: We did NOT expect an error.
+			if err != nil {
+				t.Fatalf("Delegating resolver creation failed unexpectedly with error: %v", err)
+			}
+			ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
+			defer cancel()


This test logic seems complicated enough to me that you can actually split the tests into two: go/gotip/episodes/50

eshitachandwani added 4 commits September 27, 2025 17:54

add default port

4186a66

correct bugs

2156251

change error msg

07add50

add test

abc1773

eshitachandwani added the Type: Bug label Sep 29, 2025

eshitachandwani added this to the 1.77 Release milestone Sep 29, 2025

arjan-bal reviewed Sep 29, 2025

View reviewed changes

internal/resolver/delegatingresolver/delegatingresolver.go Outdated Show resolved Hide resolved

arjan-bal assigned eshitachandwani Sep 29, 2025

correct test

0702e0c

eshitachandwani requested a review from arjan-bal September 29, 2025 09:57

eshitachandwani assigned arjan-bal and unassigned eshitachandwani Sep 29, 2025

arjan-bal assigned eshitachandwani and unassigned arjan-bal Oct 1, 2025

eshitachandwani added 3 commits October 1, 2025 17:59

add new test

2f50b78

vet

aee0066

minor changes

3d93bc1

eshitachandwani assigned arjan-bal and unassigned eshitachandwani Oct 1, 2025

arjan-bal reviewed Oct 3, 2025

View reviewed changes

arjan-bal assigned eshitachandwani and unassigned arjan-bal Oct 3, 2025

comments

80a3e98

eshitachandwani assigned arjan-bal and unassigned eshitachandwani Oct 6, 2025

eshitachandwani requested a review from arjan-bal October 6, 2025 14:15

arjan-bal assigned eshitachandwani and unassigned arjan-bal Oct 6, 2025

change conditions

c19e88d

eshitachandwani assigned arjan-bal and unassigned eshitachandwani Oct 7, 2025

easwars reviewed Oct 7, 2025

View reviewed changes

easwars assigned eshitachandwani and unassigned arjan-bal Oct 7, 2025

changes

82076ed

eshitachandwani requested a review from easwars October 8, 2025 11:58

eshitachandwani assigned easwars and arjan-bal and unassigned eshitachandwani Oct 8, 2025

		// If the port field is empty (target ends with colon), e.g. "[::1]:",
		// this is an error.

delegatingresolver: add default port to addresses #8613

Are you sure you want to change the base?

delegatingresolver: add default port to addresses #8613

Conversation

eshitachandwani commented Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eshitachandwani commented Sep 27, 2025 •

edited

Loading

codecov bot commented Sep 28, 2025 •

edited

Loading